-
-
Notifications
You must be signed in to change notification settings - Fork 772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make compile error message point to field type instead of call site for "not implemented Serialize trait" #2837
base: master
Are you sure you want to change the base?
Make compile error message point to field type instead of call site for "not implemented Serialize trait" #2837
Conversation
3d34d69
to
ed3d8be
Compare
…or not implemented Serialize trait
ed3d8be
to
9144a88
Compare
serde_derive/src/ser.rs
Outdated
@@ -1131,7 +1136,13 @@ fn serialize_struct_visitor( | |||
#func(&#field_expr, _serde::__private::ser::FlatMapSerializer(&mut __serde_state))?; | |||
} | |||
} else { | |||
let func = struct_trait.serialize_field(span); | |||
let ty = if field.attrs.serialize_with().is_none() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this affect errors when using serialize_with
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked the difference in error messages locally with serde(with)
and it doesn't change (e.g. when the signature of the given function is wrong). I can add a ui snapshot test for that today in the evening.
Also, the error for serde(with)
still points to the macro call site (not to the function in the serde(with)
attribute) as it was before my changes and after. That can also be improved by wrapping the function itself in a ::core::convert::identity<fn(#(#field_types,)*) -> __S>
which gives the type hint for the function signature and the compiler starts complaing that the function is wrong, so it points the error to the function directly. However, this unfortunatelly coerces the function to a function pointer...
As a workaround that hint could be inside of an if false
:
if false {
let _: fn( #(field_types,)*) -> __S = #fn_path;
}
// ... actual usage of `#fn_path` is unchanged
But this will just produce more errors (on inside of the if
and one at the site where #fn_path
is actually used.
One more alternative would be to generate an identity function like this:
fn type_hint<F: Fn(...) -> __S>(f: F) -> F { f }
that would give a type hint without an fn pointer coersion. But it would need to be a method on the internal SerdeWith
wrapper struct, because it needs to inherit the generics and field types from it.
Also.. it will just increase the compile times because rustc
will need to typecheck that type_hint
function additionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with
attributes fixed by me in #2558
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... So I suppose it's enough to wrap the expression of the function invocation with quote_spanned
for this to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what if I attach the span of the type itself to the parameter of serialize_field/entry
... Maybe it'll yield the same result. I'll experiment with this later today.
Otherwise I suppose #2558 should land first, because it looks like it may conflict with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I'm ready to rebase all my PRs once their fall into conflict state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked how it works. The similar behavior can be achieved if even with the expression serialize_field::<_>
where the span of _
is assigned to the span of the field type.
This is because with the explicit turbofish compiler emits an error pointing to it:
error[E0277]: the trait bound `NoImpls: _::_serde::Serialize` is not satisfied
--> crates/sandbox/src/main.rs:34:61
|
34 | _serde::ser::SerializeStruct::serialize_field::<_>(&mut __serde_state, "x2", &self.x2)?;
| ^ the trait `_::_serde::Serialize` is not implemented for `NoImpls`
|
So if its span points to smth else (like the field type), the compiler will point to the field type instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is no turbofish syntax, it's important that all parts of the field expression have the span of the field, because only then will the compiler point to the original field.
error[E0277]: the trait bound `NoImpls: _::_serde::Serialize` is not satisfied
--> crates/sandbox/src/main.rs:34:85
|
34 | _serde::ser::SerializeStruct::serialize_field(&mut __serde_state, "x2", &self.x2)?;
| --------------------------------------------- ^^^^^^^^ the trait `_::_serde::Serialize` is not implemented for `NoImpls`
| |
| required by a bound introduced by this call
For example, if all tokens &
, self
, .
and x2
have the span of the field, you'll get an error pointing to the field in this case:
error[E0277]: the trait bound `NoImpls: _::_serde::Serialize` is not satisfied
--> crates/sandbox/src/main.rs:8:5
|
8 | x2: NoImpls,
| ^^ the trait `_::_serde::Serialize` is not implemented for `NoImpls`
Alothough.. it would be better if the error pointed to the type itself. What makes this harder to achieve is that quote_spanned
doesn't change the span of the interpolated tokens. So for example, today the self
token in the field expression uses the callsite span when it's interpolated:
Lines 100 to 104 in 3415619
let self_var = if is_remote { | |
Ident::new("__self", Span::call_site()) | |
} else { | |
Ident::new("self", Span::call_site()) | |
}; |
But instead each usage of this token should change the span to the relevant field.
…ement` expression. Extract a shared function
I've updated the impl a bit and moved the logic if skipping the type hint if Unrelated observationsI've also noticed that if the user passes invalid function to To fight with that we need to re-span the field expression to assign the span of the function in Lines 1075 to 1078 in 3415619
However, just using |
Before
The error points to the call site of the
Serialize
derive:After
The error points to the type of the field directly, which makes it easier to find where exactly in the struct the type that doesn't implement
Serialize
is located.The trick is to use the type of the field in the type hint for the
serialize_field/entry
method call. Previously that call used aquote_spanned
inheriting the span of the field itself (I'm not sure that actually did anything). Now, instead of that we passserialize_field::<#ty>
, and thus the compiler knows to point to#ty
in the error message.Interestingly, the code generated by
Deserialize
already follows this pattern and thus doesn't have this problem. So this only fixesSerialize
codegen.